Refactor internal addon references to app/apps#6717
Conversation
5530a82 to
8e1e500
Compare
|
So.... I'm open to suggestions on how to break this enormous PR up lol. I considered asking the AI to go one module at a time but it would still cascade a ton. Since it would then have to change references elsewhere and end up making a lot of sections of confusing code that had half apps references and half addons references. I figured I'd start with what it got as long as the tests passed and we'd decide where to go from there. |
I agree. It's indeed enormous but on the other hand the changes are quite straight-forward to review - I think it's fine to do it in one shot rather that doing it in smaller chunks. I can imagine the changes could have effect out of the scope of a single module that you'd not realize when reviewing what's basically a |
sairon
left a comment
There was a problem hiding this comment.
Looking into the changes a bit deeper, there could be done some improvements for easier review (which should be easy to be performed by AI). Ideally splitting it into several commits - hunks that only change comments and docstrings (there are tons of such changes that we don't care about much) in one changeset, and changes in the code itself in another - on those we can better focus during the review.
I also don't agree this should be left out:
File and directory names (supervisor/addons/, addons/addon.py, etc.) — filesystem paths
AFAIK the module/package names and paths are only used internally by Supervisor - we don't need to fulfill any contract by keeping the names the same there.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
@sairon we do actually 🙈 But only in one place, resolution manager. Here's where the slugs of checks, evaluations and fixups come from currently: supervisor/supervisor/resolution/checks/base.py Lines 53 to 56 in 321b692 supervisor/supervisor/resolution/evaluations/base.py Lines 45 to 48 in 321b692 supervisor/supervisor/resolution/fixups/base.py Lines 93 to 96 in 321b692 And these do definitely make their way into the API contract. The fix is quite simple - make this field abstract and have each class override it with a hard-coded value. But since coding changes were required for this one (not just find and replace) and it was quite easy to carve this particular bit off into a separate PR I figured I'd leave it for follow-up. |
8e1e500 to
c51bb19
Compare
|
@sairon ok PR split into 3 commits:
What it actually did was took a chunk out of its original script and made a new docstrings and comments only one. Then it just ran the original one as is since its idempotent anyway. I had it add the new script to the details in the expand pane as well. |
c51bb19 to
15ada2c
Compare
sairon
left a comment
There was a problem hiding this comment.
Well, it's still PITA to review but at least the change traceability is better 🙈 I can't say I've review it thoroughly but it I don't see anything strange, the changes are generally mechanical and tests pass, so let's go with that 👍
Updates all docstrings and inline comments across supervisor/ and tests/ to use the new app/apps terminology. No runtime behaviour is changed by this commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Renames all internal Python identifiers from addon/addons to app/apps: - Variable and argument names - Function and method names - Class names (Addon→App, AddonManager→AppManager, DockerAddon→DockerApp, all exception, check, and fixup classes, etc.) - String literals used as Python identifiers (pytest fixtures, parametrize param names, patch.object attribute strings, URL route match_info keys) External API contracts are preserved: JSON keys, error codes, discovery protocol fields, TypedDict/attr.s field names. Import module paths (supervisor/addons/) are also unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The external API accepts `addons` as the request body key (since
ATTR_APPS = "addons"), but do_backup_partial and do_restore_partial
now take an `apps` parameter after the rename. The **body expansion
in both endpoints would pass `addons=...` causing a TypeError.
Remap the key before expansion in both backup_partial and
restore_partial:
if ATTR_APPS in body:
body["apps"] = body.pop(ATTR_APPS)
Also adds test_restore_partial_with_addons_key to verify the restore
path correctly receives apps= when addons is passed in the request
body. This path had no existing test coverage.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
15ada2c to
cad44b2
Compare
|
Converting to draft for now since we want to do a release which includes #6590 before merging this. |
Co-authored-by: Stefan Agner <stefan@agner.ch>
agners
left a comment
There was a problem hiding this comment.
LGTM, also runtime tested locally, worked fine 👍 .
Summary
This PR renames all internal code references from
addon/addonstoapp/appsto make the codebase consistent with the user-facing rebranding done in #6696. The goal is to make the codebase clearer for future contributors by aligning internal naming with the new "apps" terminology.What changed
addon→app,addons→appsget_addon_logs→get_app_logsAddon→App,AddonManager→AppManager,DockerAddon→DockerApp, all exception classes, check/fixup classes, etc.What was deliberately NOT changed
supervisor/addons/,addons/addon.py, etc.) — filesystem paths"addons","addon"in responses) —ATTR_APPS = "addons"keeps its string valueBackendAuthRequest(TypedDict).addon— used by HA Core auth APIMessage(@attr.s).addon— used by the discovery protocolextra_fieldsdict keys andmessage_templateplaceholders in exceptions — returned verbatim in API error responses"addon_build_dockerfile_missing_error") — external API contractManual fix: backup/restore partial key remapping
The backup and restore partial API endpoints pass
**bodydirectly todo_backup_partial/do_restore_partial. The external API still acceptsaddonsas the request key (sinceATTR_APPS = "addons"), but the internal function parameters were renamed toapps. A remapping step was added before the**bodyexpansion:A test (
test_restore_partial_with_addons_key) was also added to cover this path, which previously had no test.Commit structure
The changes are split into three commits for reviewability:
Refactoring scripts used to perform the renames
The renames were performed in two passes using Python tokenizer-based scripts. Using the
tokenizemodule (rather than regex) ensures identifiers, docstrings, f-string expressions, and import paths are all handled correctly. Both scripts are idempotent.Pass 1 — docstrings and comments only
Run first to produce commit 1. Only touches
COMMENTtokens and triple-quoted docstringSTRINGtokens; all code identifiers are left unchanged.Pass 2 — code identifiers (run after pass 1)
Handles NAME tokens (identifiers), f-string expressions, and non-docstring string literals. Import module paths are protected. External interface fields (
TypedDict,@attr.s) are detected and reverted after the token pass.